Repository navigation
[Bugfix][MiniCPM-o] Restore the duplex tests removed in #7413 - #7872
chickeyton wants to merge 4 commits into
Conversation
…st in vllm-project#7413 vllm-project#7413 rebuilt the duplex serving stack around engine-resident sessions and dropped the per-response request timing that vllm-project#7242 had added for the MiniCPM-o Omni-DuplexEval / OmniInteract benchmarks: the engine no longer emitted ``response_request_metrics``, so ``DuplexClient`` and the benchmark summaries silently fell back to the client's receive clock, and the TPOT weighting regressed to counting a one-token segment as one interval. Port the timing into the engine session: the model-turn request start is recorded when the model channel submits the append, the first text / audio of the response fix TTFT / TTFP, and the numbers ride response.created (under response.metadata.duplex_event) and every speak / audio delta (under metadata.vllm_omni), exactly where the client already reads them. Restore the interval-weighted TPOT mean. Re-home the MiniCPM-o plugin policy unit tests deleted with tests/engine/duplex/test_duplex_runtime.py, make the duplex e2e gate assert the server-side anchor, fix the pre-existing mypy errors in the two touched test files, and drop three doc passages that still describe the session-per-request chat adapter removed before vllm-project#7413 merged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: chickeyton <ngton2014@gmail.com>
Pre-check report (
|
|
This PR appears to belong to: docs/design/module/engine_orchestration.md, docs/design/module/observability.md. Module owners: @tzhouam @fake0fan @lishunyang12 Routing: @tzhouam via module of the changed files, CODEOWNERS; @fake0fan via module of the changed files; @lishunyang12 via module of the changed files @chickeyton, please review your own changes and leave a short self-review comment describing what you checked. PRs without author self-review may not be assigned a reviewer. Please take a look when you have a chance. If you would like an automated review, mention @vllm-omni-review-bot in a comment. |
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
|
@yenuo26 PTAL |
Omni ReviewBot: no human activity for 7 days@chickeyton this pull request has had no human commit, comment or review since 2026-09-20. Please confirm the current plan and next step. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
Omni ReviewBot: no human activity for 14 days@chickeyton this pull request has had no human commit, comment or review since 2026-09-20. Please consider marking this PR as draft until work can resume. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
Omni ReviewBot routing recordAssigned Strict on cursor (cursor-grok-4.6-high) under experiment |
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
3 actionable finding(s).
CI at
e3f0acf4776d(2026-10-10T09:46:59.551009+00:00): required check(s) blocking:buildkite/vllm-omni(missing).
Full review analysis
Scan:
| Category | Result |
|---|---|
| Tests / verification | 3 finding(s) below |
| Security | no finding reported |
| Docs / comments | no finding reported |
| Behavior / compatibility | no finding reported |
| Correctness | no finding reported |
Validated:
- [resolved] Chat-fallback / session-per-request docs: README.md:158-160 and api_server.py:653-656 now agree (ordinary chat alongside live sessions). Residual: realtime_duplex_api.md:485-517 still mixes /v1/chat/completions audio_metadata with duplex-route playback under one heading — no product break.
- [resolved] barge_in/close use _clear_response_timing(drop_pending=True) engine_session.py:1433/1449; residual: LISTEN without an active response does not barge-in, so later chunks keep overwriting the pending start
- [resolved] client already preferred server metrics (tests/clients/test_duplex_client.py:1133 test_event_collector_prefers_server_request_start_for_ttf); residual: Qwen3 precreate created event and experimental/fullduplex/client.py.
- [resolved] failed-append mark-before-RPC: latest-wins in by_turn; precreate fail_precreated_response + end_response clears active timing (drop_pending=False). Residual: start is recorded before on_append_accepted.
- [resolved] README:158 ordinary chat on DuplexOmni — rejected no_issue; full_duplex_api.md:34-38 already matches
- [claim-verified] DuplexChatCompletionsAdapter / test_chat_completions_adapter.py have no remaining in-repo refs; api_server.py:653-656 is ordinary chat on DuplexOmni when supports_chat_completions.
3 actionable finding(s).
Verdict: REQUEST CHANGES
Findings
- **[P1] While no response is bound,
mark_model_turn_request_startedlast-winsreques…** —vllm_omni/engine/duplex/session/engine_session.py`
Evidence for While no response is bound, `mark_model_turn_request_started` last-wins `reques…
While no response is bound, mark_model_turn_request_started last-wins request_started_at_s_by_turn[turn_id] on every call, and append_runtime_input stamps that time before the append RPC (model_channel.py:170) even though the comment above still says timing commits only if the append submitted. MiniCPM-o 4.5 (supports_core_resumable_request=True) keeps one Stage0 id (stage_request_id omits turn); later chunks are submit_update (already_submitted=True), and a pre-speech LISTEN does not begin_response or complete_model_turn, so silence units overwrite the pending start. begin_response then pops that last stamp, so Omni-DuplexEval / the seeded-text e2e (200ms silences) advertise measurement_origin "native model-turn request execution start" while measuring from the last pre-speech append. First-win the pending map (setdefault) and stamp only after a successful first submit so a failed later chunk cannot poison the ledger. Flip test_response_timing_binds_latest_request_start_for_model_turn (it pins 200ms from 11.0, not 1200ms from 10.0) and add a two-append runner case; _assert_request_metrics only checks source == server_request_start_and_client_receive and ttft_ms >= 0, which cannot catch last-unit bias.
Evidence: Trigger: MiniCPM-o 4.5 native duplex keeps one Stage0 request; each pre-speech 200ms silence/LISTEN append last-wins the unbound pending start, then the first speak pops that last time as TTFT/TTFP while advertising request-execution start. engine_session.py:593-600 def mark_model_turn_request_started / the latest one winning. engine_session.py:608 self._response.request_started_at_s_by_turn[turn_id] = started_at_s. engine_session.py:778-781 self._response.active_response_request_started_at_s = self._response.request_started_at_s_by_turn.pop(self._response.active_response_turn_id, None,). model_channel.py:165-170 the acceptance callback commits timing state only if the append actually submitted. then session.mark_model_turn_request_started(helpers.append_fence(session, payload).turn_id, submit_time) before the RPC, with no rollback on failure. Unchanged by this diff, present in the PR-time tree: minicpmo_4_5/duplex/capabilities.py:29 supports_core_resumable_request=True; manager.py:411-415 if not resumable: role += f"-turn{fence.turn_id}" so resumable omits turn; model_channel.py:227 already_submitted = session.stage_request_submitted(stage_id, request_id); duplex_orchestrator.py:355-358 if submission.already_submitted: replica_id = await pool.submit_update(...) else submit_initial. Adverse effect: _on_model_listen with no active_response_id emits response.listen without begin_response/complete_model_turn, so later units overwrite turn 0; tests/engine/duplex/test_engine_session.py:698-703 stamps 10.0 then 11.0 and asserts ttft_ms == 200.0 from 11.0, not 1200ms from 10.0; tests/e2e/online_serving/test_minicpmo_4_5_duplex.py:52-59 only requires ttft_ms >= 0 and source == "server_request_start_and_client_receive" while sending 200ms silences (lines 255-270).
Suggestion: self._response.request_started_at_s_by_turn.setdefault(turn_id, started_at_s)
Evidence for docs/cli/bench/serve.md:321 (unchanged by this PR) still says per-response TTFT…
docs/cli/bench/serve.md:321 (unchanged by this PR) still says per-response TTFT/TTFP start when the server begins executing the native model-turn request. That is false for the OmniInteract 200 ms multi-append path on the same page: every append_runtime_input marks at model_channel.py:170, the pending ledger latest-wins at engine_session.py:608, and later chunks reuse the already-submitted per-turn stage request. Rewrite 321 to say TTFT/TTFP bind to the latest pre-response append mark, not request-execution start.
Evidence: Trigger: unchanged by this diff, present in the PR-time tree: docs/cli/bench/serve.md:305 Audio is replayed as 16 kHz PCM16 in 200 ms chunks and video at 1 FPS with real-time pacing. Unmet requirement (same file, unchanged): docs/cli/bench/serve.md:321 Per-response TTFT and TTFP start when the server begins executing the native model-turn request that owns the response. In this diff, every chunk is treated as a start: model_channel.py:170 session.mark_model_turn_request_started(helpers.append_fence(session, payload).turn_id, submit_time). Pending latest-wins (this diff): engine_session.py:608 self._response.request_started_at_s_by_turn[turn_id] = started_at_s. Later chunks are not a new native execution — unchanged by this diff: manager.py:411-415 def stage_request_id(fence: DuplexFence, *, stage_id: int, resumable: bool = True) -> str: / role = f"stage{stage_id}" (turn suffix only when not resumable); model_channel.py:227 already_submitted = session.stage_request_submitted(stage_id, request_id); engine_session.py:309 resource.submitted = True. Pinned by this diff: tests/engine/duplex/test_engine_session.py:698-703 marks 10.0 then 11.0, assert first["ttft_ms"] == pytest.approx(200.0) (from 11.0, not 10.0). Adverse effect: OmniInteract TTFT/TTFP are last pre-response append time, not first native request-execution start, so serve.md:321 states a metric contract the restored ledger does not keep.
🤖 This review was generated by InferMatrix Copilot, an open-source repo-maintenance agent for PR review, CI debugging and issue triage. Try it on your own repo, and ⭐ star it if it helped!
| response, fails that fallback response with `response_error`. | ||
|
|
||
| In this Chat fallback path, `response.done.response.metadata.playback` reports: | ||
| On the duplex route the audio delta itself carries the output `format` and |
There was a problem hiding this comment.
[P2] This hunk rewrites duplex Chat-audio metadata (format / sample_rate_hz / `m…
Evidence and suggested fix
This hunk rewrites duplex Chat-audio metadata (format / sample_rate_hz / metadata.audio_duration_ms) while the same PR restores response_request_metrics on response.created and every audio delta, but two in-page contracts still describe the old client-only story. L270 says timing_summary reports client-observed first-text / first-audio latency; L438's Tier 2 response.output_audio.delta row lists only format/sample_rate/duration/playback. The page never names response_request_metrics. Unchanged by this diff, EventCollector.timing_summary already prefers server ttft_ms/ttfp_ms (source: server_request_start_and_client_receive) — the Omni-DuplexEval criterion this restore is for. Update those two passages to name the field.
Evidence: Trigger: this PR attaches response_request_metrics on the duplex wire (vllm_omni/engine/duplex/session/model_channel.py:790 created_payload["response_request_metrics"] = dict(response_request_metrics) and the same dict on audio-delta metadata.vllm_omni) and rewrites this page's Chat-audio section (docs/serving/realtime_duplex_api.md:504 On the duplex route the audio delta itself carries the output \format` and). Adverse effect / unmet requirement: a reader of the same public page is still told the client-only contract, so they will misread Omni-DuplexEval TTFT/TTFP as receive-clock numbers. Unchanged by this diff, present in the PR-time tree: docs/serving/realtime_duplex_api.md:270``timing_summaryreports client-observed first-text / first-audio latency, ``;docs/serving/realtime_duplex_api.md:438``response.output_audio.delta|format, sample_rate_hz, metadata{session_id, epoch, model_speak, end_of_turn, audio_duration_ms, audio_text_marks, playback}`` (file has zeroresponse_request_metricsmentions). Unchanged by this diff, present in the PR-time tree:vllm_omni/clients/duplex.py:1753-1756 "server_request_start_and_client_receive" if server_ttft_ms is not None or server_ttfp_ms is not None else "client_monotonic_receive"and:1760-1761 server_ttft_ms if server_ttft_ms is not None else (...client receive...); vllm_omni/benchmarks/duplex_session_metrics.py:69 Per-response TTFT/TTFP prefer server ``response_request_metrics```.
Suggestion: timing_summary prefers per-response TTFT/TTFP from server response_request_metrics when response.created or an audio delta carries them (source: server_request_start_and_client_receive); otherwise it falls back to client-observed first-text / first-audio latency.
Omni ReviewBot: finding feedback**[p1] While no response is bound, See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement. |
What broke
Since #7413 a duplex session never reports the server-side per-response TTFT/TTFP that #7242 added for the MiniCPM-o Omni-DuplexEval / OmniInteract benchmarks.
response.createdand the audio deltas carry noresponse_request_metrics, sovllm_omni.clients.duplex.EventCollectorandbenchmarks/duplex_session_metrics.pysilently fall back to the client's receive clock ("source": "client_monotonic_receive"), whiledocs/cli/bench/serve.mdstill promises "Per-response TTFT and TTFP start when the server begins executing the native model-turn request". The same commit's TPOT weighting fix was lost too: a one-token segment is weighted as one interval again and skews the response mean.Repro
The strengthened e2e gate fails on
mainthe same way: every response'srequest_metrics.sourceisclient_monotonic_receiveinstead ofserver_request_start_and_client_receive.CUDA_VISIBLE_DEVICES=0 pytest -s -v tests/e2e/online_serving/test_minicpmo_4_5_duplex.py \ tests/e2e/online_serving/test_duplex_client_live.py -m "core_model and cuda" --run-level core_modelRoot cause
#7413 replaced
entrypoints/duplex/{protocol,runtime_bridge,session_runner}.pywith the engine-residentDuplexEngineSession/ModelChannel, and the port did not carry overmark_model_turn_request_started,mark_response_first_outputs, theresponse_request_metricsattachment, or themax(tokens - 1, 0)TPOT weight. The client side of #7242 survived, so the feature degraded silently instead of failing.Fix
engine_session.py: per-turn request-start ledger onResponseState,mark_model_turn_request_started/mark_response_first_outputs, binding atbegin_response, pruning atcomplete_model_turn, resets on end / barge-in / close, and the interval-weighted TPOT mean.model_channel.py: anchors the clock when the append is submitted and attaches the metrics toresponse.created(lands underresponse.metadata.duplex_event) and to every speak / audio delta (metadata.vllm_omni), the two places the client already reads.tests/engine/duplex/test_duplex_runtime.py, and the duplex e2e gate now asserts the server-side anchor. The 11 pre-existing mypy-3.10 errors in the two touched test files are fixed so pre-commit passes.DuplexChatCompletionsAdapterthat was removed before [Core][Frontend] Unified Full-duplex Framework #7413 merged; they now match the chat route that exists.Tests restored
Thirteen tests across four files: ten restore tests #7413 deleted, three are new.
From
tests/entrypoints/openai_api/test_duplex_handler.py(deleted by #7413), now intests/engine/duplex/test_engine_session.pytest_response_timing_binds_latest_request_start_for_model_turnDuplexEngineSessiontest_response_tpot_fallback_ignores_single_token_segmenttest_response_tpot_keeps_token_weighted_value_when_itls_existFrom the same file, now in
tests/engine/duplex/test_session_runner.pytest_continuous_response_metrics_accumulate_only_owned_model_units(theresponse_request_metricshalf)test_response_request_metrics_are_measured_from_the_model_turn_request_start, driven through the runner harness with the real MiniCPM-o pluginFrom
tests/engine/duplex/test_duplex_runtime.py(deleted by #7413), now in the newtests/model_executor/models/minicpmo_4_5/duplex/test_plugin_policy.pytest_minicpmo_extension_owns_stage_sampling_overridestest_plugin_owns_stage_sampling_overrides_without_mutating_defaultstest_minicpmo_output_decision_uses_raw_streaming_token_snapshottest_listen_decision_uses_the_raw_streaming_token_snapshottest_minicpmo_output_decision_ignores_output_level_token_history(x2 params)test_listen_decision_ignores_output_level_token_historytest_minicpmo_output_decision_uses_completion_token_ids(x2 params)test_listen_decision_uses_completion_token_idstest_minicpmo_output_decision_uses_completion_stop_reasontest_listen_decision_uses_completion_stop_reasontest_duplex_scheduler_token_budget_estimates_pcm_slotstest_scheduler_token_budget_estimates_pcm_slotstest_duplex_scheduler_token_budget_ignores_client_budget_fieldstest_scheduler_token_budget_ignores_client_budget_fieldsNew, no deleted counterpart
test_response_timing_is_empty_without_a_request_start_and_is_scoped_to_one_response(test_engine_session.py): lifecycle of the timing state across end, turn completion, and barge-in.test_a_speak_segment_is_not_a_listen_decision(test_plugin_policy.py): negative case for the listen decision.tests/e2e/online_serving/test_minicpmo_4_5_duplex.py:_assert_request_metricsnow requiressource == "server_request_start_and_client_receive"and the native request-start measurement origin, so the CI e2e gate catches this regression.Deliberately not restored
test_duplex_scheduler_token_budget_one_frame_is_one_block,..._matches_stage0_hd_slicing,..._unreadable_base_frame_keeps_hd_fallbackandtest_duplex_hd_slice_count_matches_official_gridfromtest_duplex_runtime.py: [Bugfix][MiniCPM-o] Size the duplex HD slice reservation from the frame #7654 re-covers them intests/model_executor/models/minicpmo_4_5/duplex/test_scheduler_token_budget.py.test_minicpmo_*handler tests, which targeted the serving architecture [Core][Frontend] Unified Full-duplex Framework #7413 removed.Validation (1x NVIDIA L20X, vllm 0.29.0, torch 2.13.0+cu129)
pre-commit run --files <changed>(all hooks incl. mypy-3.10)tests/engine/duplex tests/clients tests/model_executor/models/minicpmo_4_5 tests/benchmarks/{test_omniinteract.py,patch,metrics} tests/entrypoints/duplex tests/engine/test_duplex_orchestrator.py tests/engine/test_duplex_omni_engine.py tests/entrypoints/test_duplex_omni.py tests/entrypoints/openai_api/test_duplex_api_server.py tests/entrypoints/openai/test_duplex_session_attachment.py tests/worker/test_native_duplex_hooks.py tests/protocol tests/engine/test_duplex_import_boundary.py tests/engine/test_output_processor.py-m "core_model and cuda" --run-level core_modeltest_engine_session.py,test_session_runner.py,test_plugin_policy.py)Not restored on purpose: the frame-size-aware vision slot budget from #7271 was also dropped by #7413 but has since been re-fixed by #7654; the duplex Seed-TTS perf baselines removed in #7413 are left out because the native per-unit path changed.
🤖 Generated with Claude Code